Issue 36702 asset picker - #36944

Claude finished @nicobytes's task in 2m 30s —— View job

Re-review at 2801a61e

Rechecked every prior finding against the code at this HEAD (not commit messages), and scanned the diff for new issues. The only source change since the last full review is the bundle-budget raise (157ef01b) plus two main merges — no new logic. Findings below.

New Issues

None. (The bundle-budget raise in apps/dotcms-block-editor/project.json is a real tradeoff, not a defect — see note at the end.)

Existing (still open)

• 🟠 High: libs/ui/src/lib/components/dot-asset-picker/store/features/with-asset-folder-tree.feature.ts:216 — publish() still re-points by key and lands on null when the key is absent (findNodeByKey(folders, selectedKey) ?? null). After a sidebar folder search, searchFoldersInBrowsingSite deliberately leaves selectedNode pointing outside the published tree, so the next mutateNode → publish (expand / "Load more") nulls it → $targetFolder becomes undefined → #uploadByBaseType silently drops the upload at the site root while the list still shows the searched folder. This is the one genuinely user-facing bug still open. You proposed two fixes; option (1) — keep the previous node when the key misses — matches the documented intent of the search path. Fix this →

• 🟡 Medium: libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.spec.ts:1 — still 37 lines, no longer imports tree-folder.utils, while tree-folder.utils.ts is byte-identical to main and still consumed by store/features/sidebar/withSidebar.ts and utils/functions.ts. generateAllParentPaths / createTreeNode / buildTreeFolderNodes are now uncovered live logic — coverage was dropped, not moved. Largest gap in the PR.

• 🟡 Medium: libs/portlets/dot-content-drive/ui/src/lib/shared/models.ts:14 + dot-content-drive-shell.component.ts:1128-1133 — DotContentDriveUploadFiles/DotContentDriveMoveItems are now aliases of DotUploadFiles (optional targetFolder: TreeNodeData), but getMoveMetadata still reads event.targetFolder.hostname / .path unguarded. onRequestUpload was narrowed on LOAD_MORE_NODE_TYPE; getMoveMetadata was not, so the two are inconsistent and a load-more node yields //undefined/. Either narrow getMoveMetadata the same way or restore a Content-Drive-specific alias that requires folder data.

• 🟡 Medium: libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.ts:313 — this.$selection() !== undefined (was ??) while the input was widened to accept null. A controlled consumer binding null for "nothing selected" — the natural shape for the new selectionMode="single" — gets a permanently empty selection. Latent (both in-repo consumers pass arrays), but a trap for the exact mode this PR adds. Reverting to ?? is a one-liner.

• 🟡 Medium: libs/edit-content/.../dot-file-field/dot-file-field.component.ts — "Select Existing File" is still a silent no-op when getCurrentSite() returns falsy or errors (error: () => { /* noop */ }). Same silent path was later added to EditorModalService and DotWysiwygPluginService, so this is one consistent UX decision across three entry points, not a one-line fix — worth deciding on before merge.

• 🔵 Low: libs/ui/src/lib/components/dot-workflow-push-publish/dot-workflow-push-publish.component.html:95 — 'Change' → 'change' renders lowercase (change=change vs Change=Change in Language.properties). Confirmed merge damage from 5624a082 (main has 'Change'), same commit that dropped the SiteSearchAPIImpl passthrough. One-character revert. Fix this →

• 🔵 Low: libs/portlets/dot-content-drive/ui/src/index.ts:1 — two import paths still resolve the same classes (@dotcms/ui and @dotcms/portlets/content-drive/ui). Naming/ownership cleanup, fine as a follow-up.

Resolved

• ✅ dot-asset-picker.component.ts — NullInjectorError/NG0201 in the legacy Dojo host. DotHttpErrorManagerService removed from all 6 store call sites; failures now recorded as requestError state and toasted by the component; legacy-host spec un-skipped.
• ✅ SiteSearchAPIImpl.java:263 — DotDataException passthrough restored (file byte-identical to main).
• ✅ dot-file-field.component.ts — double-open race guarded with #assetPickerPending, released on close/error; three tests added.
• ✅ with-asset-folder-tree.feature.ts — searchFoldersInBrowsingSite re-adds the browsing site to roots when the site filter drops it.
• ✅ with-asset-folder-tree.feature.ts:317 — TreeLoadResult absent-key contract honored via conditional spread; loadFolders keeps success work in tap before catchError (ERROR no longer masked by LOADED).
• ✅ dot-asset-picker.component.ts — confirm() has an error: handler that toasts and keeps the dialog open.
• ✅ dot-asset-picker.store.ts:150-154 — setSearch clears path and selectRootNode(), so the highlight and upload target follow the widened scope.
• ✅ libs/ui/src/index.ts + dot-file-field.component.ts:887 — File/Image "Select Existing" now opens DotAssetPickerComponent; DotBrowserSelectorComponent no longer referenced.

Note on the bundle budget (157ef01b)

The raise (warn 3.5→4.25 MB, error 4→4.5 MB) is a genuine growth, not a moved budget: EditorModalService holds a static DotAssetPickerComponent reference for dialogService.open, pulling the picker subtree into the initial chunk of every @dotcms/new-block-editor consumer, dotcms-ui included. The lazy-import() route you described keeps the original budget and is semantically correct (the picker only opens on user action). Not blocking, but worth doing before this compounds.

Blocking before merge: the 🟠 publish() upload-misplacement bug. Everything else is non-blocking; the two 🔵 merge-damage items ('change' here, and you already fixed the Java one) support your call for a manual pass over git diff origin/main on backend/CI files before merge.

Review feedback status — rechecked at e21bd64756

Went through every Critical/High finding from @zJaaal, @adrianjm-dotCMS, Copilot and the claude[bot] pass. Below is what changed since the last review, verified against the code at this HEAD rather than from the commit messages.

✅ Addressed

🟠 SiteSearchAPIImpl — merge dropped main's DotDataException passthrough (dfeca53acc)
Restored the } catch (DotDataException e) { throw e; } in defaultIndexName(). The file is now byte-identical to origin/main, so merging this PR no longer reverts the #36983 fix, and it is consistent again with isDefaultIndex() / getAggregations().

🟠 Async dialog open can stack two pickers (dfeca53acc)
Added an #assetPickerPending flag in dot-file-field.component.ts. Notes on the shape of the fix:

• #dialogRef could not serve as the guard: it is shared with the other three dialogs this component opens and is never reset on close, so checking it would have blocked the picker permanently after any of them ran.
• The flag is released on error, when no site resolves, and on every dialog close. That last one required dropping the filter(file => !!file) from the onClose subscription — with the filter in place, cancelling never released the guard and the button would have died for the rest of the session.
• Three tests added in dot-edit-content-file-field.component.spec.ts: double click while the site lookup is in flight, re-open after close, and retry after a failed lookup.

The same race existed on the two callers added later in this PR (EditorModalService for the Story Block, DotWysiwygPluginService for the WYSIWYG field); both already guard with an equivalent pending flag, so all three entry points are consistent now.

⚠️ Partially addressed — please re-read this one

🔴 Missing providers / picker cannot construct in the legacy Dojo binary-field host

b27ab7557b adds DotHttpErrorManagerService and DotContentTypeService to the picker's providers, as suggested. That is necessary but not sufficient, so this finding should stay open.

I added a regression spec that constructs DotAssetPickerComponent with only the provider set the custom-element host actually has (dot-asset-picker.component.legacy-host.spec.ts, ec5aaccfca). It still fails:

NG0201: No provider found for `DotAlertConfirmService`

DotContentTypeService is satisfied — it only needs HttpClient. DotHttpErrorManagerService is not; it transitively pulls in:

DotHttpErrorManagerService
  ├── DotAlertConfirmService   → ConfirmationService          ← fails here now
  ├── DotMessageDisplayService → DotRouterService → Router, DotEventsSocket
  └── DotRouterService         → Router

apps/dotcms-binary-field-builder bootstraps without a Router — which is the same reason GlobalStore was deliberately kept out of DotFileFieldComponent. The chain therefore cannot be closed with component-level plumbing at all: provideRouter returns EnvironmentProviders and cannot go in a component's providers array.

The fix that actually closes it is to drop the DotHttpErrorManagerService dependency from the picker's store features (6 call sites across with-asset-browse / with-asset-folder-tree) and report errors through the MessageService toast the picker already owns — it does exactly that for upload and confirm failures today. The spec is committed skipped, with the chain documented, to be un-skipped as part of that change.

❌ Still open

One thing worth a separate look

Two of the findings above (SiteSearchAPIImpl, and the 'Change' → 'change' i18n key) are both merge-resolution damage from the same commit (5624a082d4), not intentional changes — main has 'Change' on that line. Since two independent regressions came in through one automated conflict resolution, a manual pass over the backend and CI files in git diff origin/main before merge seems worthwhile; I was not able to complete that sweep myself.

Claude finished @nicobytes's task in 1m 15s —— View job

I'll analyze this and get back to you.

Two changes pushed that reviewers should know about (852bf9ca12, 157ef01ba6)

1. The 🔴 provider finding is now genuinely closed

Details in the thread. Short version: DotHttpErrorManagerService is out of the picker's store features — the store records failures as state, the component toasts them — and the legacy-Dojo-host spec is un-skipped and passing. My earlier "please keep this open" comment no longer applies.

2. I raised a bundle budget, which nobody asked for

nx run dotcms-block-editor:build:production was failing:

✘ [ERROR] bundle initial exceeded maximum budget. Budget 4.00 MB was not met by 80.43 kB with a total of 4.08 MB.

apps/dotcms-block-editor/project.json is byte-identical to main, so the budget was not moved — the bundle grew. Cause is this PR: wiring the Story Block to the picker means EditorModalService holds a static reference to DotAssetPickerComponent (dialogService.open needs the class), so the picker and its subtree — folder-list table, folder tree, splitter, content-type filter, Content Drive store — land in the initial chunk. It replaced DotBrowserSelectorComponent, which was considerably lighter.

I raised warning 3.5mb → 4.25mb and error 4mb → 4.5mb to unblock the build.

Flagging it because it is a budget raise, not an optimization, and it is not confined to the demo app: every consumer of @dotcms/new-block-editor now ships the picker in its initial bundle, dotcms-ui included.

The alternative is loading the picker through a dynamic import() in openAssetPicker, moving its subtree to a lazy chunk and keeping the original budget. The picker only ever opens on user action, so lazy is semantically right. I did not do it because I could not verify it is sufficient on its own — @dotcms/ui is already pulled in statically for DotAIImagePromptComponent, though esbuild tree-shakes per symbol so the picker's subtree should still move.

Two caveats on my own diagnosis, so nobody takes it as measured fact:

• I did not confirm whether this build passes on main. I attributed the growth by reasoning about the dependency graph, not by an A/B measurement — my attempt to measure it by stubbing the import did not come out clean and I reverted it rather than leave the tree dirty.
• I did not measure the picker's exact contribution.

Happy to do the lazy-import route instead if you would rather not carry the raise.